Skip to content

Fix failing checks#443

Closed
sjentsch wants to merge 5 commits intojamovi:mainfrom
sjentsch:fix_failingchecks
Closed

Fix failing checks#443
sjentsch wants to merge 5 commits intojamovi:mainfrom
sjentsch:fix_failingchecks

Conversation

@sjentsch
Copy link
Copy Markdown
Member

@sjentsch sjentsch commented Jan 16, 2026

These changes should take care of the failing checks on GitHub (oldrel and devel). Ironically, they do only partially (see below why).

To prevent the checks for devel from failing, the Durbin(-Conover)-test was re-implemented based upon NIST and the Wikipedia entry for the test. This removes the dependency on PMCMR which is deprecated (and will be removed with the next version of R). devtools::test() for anovarmnp is successful. I conducted further tests with examples from the internet, comparing it with the current version of jamovi (using PMCMR) and the re-implementation, ensuring the same results.

The GitHub actions for macos-latest and ubuntu-oldrel fail (because of install.packages issue). I can't check the macos-issue, however, when trying the code out in a docker / rocker container with R 4.4 (~ ubuntu-oldrel) it works without problems.

@jonathon-love
Copy link
Copy Markdown
Member

jonathon-love commented Jan 19, 2026

The GitHub actions for macos-latest and ubuntu-oldrel fail

i think we'll probably just standardise on windows, and only bother running the windows tests ... the macOS and linux tests often fail for reasons outside of our control ...

raviselker
raviselker reviewed Jan 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@raviselker raviselker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions and questions, but looks good!

Comment thread R/anovarmnp.b.R Outdated
Comment thread R/descriptives.b.R Outdated
Comment thread R/anovarmnp.b.R Outdated
@raviselker
Copy link
Copy Markdown
Member

raviselker commented Jan 21, 2026

The GitHub actgions for the ubuntu-latest (devel) fails on the actual tests. Any idea why this is happening?

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Jan 21, 2026

Regd.: The GitHub actgions for the ubuntu-latest (devel) fails on the actual tests. Any idea why this is happening?

No, not yet. I also just observed it. Strangely enough, the error did not happen in my branch (which was branched from an older version of jmv; see screenshot below).
It is unlikely that it has to do with a change to R-devel happened in that period between my submission and the PR (both on Jan 16). It is more likely that it has to do with code you added recently (fix_failingchecks is 15 commits behind main).

image

@shun2wang
Copy link
Copy Markdown
Contributor

shun2wang commented Jan 22, 2026

I think I can fix failure on macOS, but I have no access to push on your PR so I made another PR see:#444

@shun2wang
Copy link
Copy Markdown
Contributor

shun2wang commented Jan 22, 2026 •
edited
Loading

Failed on Linux older version it is becasue RcppEigen package build error. I guess it won't compile against R 4.4.x.

@jonathon-love
Copy link
Copy Markdown
Member

jonathon-love commented Jan 22, 2026

yeah, i think testing across all these OSes doesn't actually buy us anything ... it doesn't increase the likelihood that something problematic in our code will be identified ... all it does is raise the maintenance burden.

The GitHub actgions for the ubuntu-latest (devel) fails on the actual tests. Any idea why this is happening?

i think i'm in the "who knows? who cares?" camp ... it's almost definitely because of something in R which needs babying, and almost definitely not anything that we need to care about.

@sjentsch
Moved calculation of Durbin-Conover-Test into a separate function (ca…
5a04cc1 
…lcDurbin), added option for p-adjustment, and added further unit tests
@sjentsch
Reverted earlier commit: 'Adapt to changes in the handling of the tab…
f84579a 
…le function (changes in R 4.6)'
@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 25, 2026

@raviselker: I resolved the comments / suggestions that you made previously. Please have another look!

raviselker
raviselker reviewed Feb 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@raviselker raviselker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more comment. Rest looks good to me!

Comment thread tests/testthat/testanovarmnp.R Outdated
@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 26, 2026

@raviselker: I dealt with your comment.

raviselker
raviselker approved these changes Feb 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@raviselker raviselker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jonathon-love
Copy link
Copy Markdown
Member

jonathon-love commented Feb 26, 2026

OK, i've squashed and merged this, but i did leave a small change to descriptives behind ...

If this is a fix for something, let me know and i'll merge it as a separate commit.

# before
counts <- do.call("[", c(list(freq), grid[row, ]))

# after
counts <- as.numeric(do.call("[", c(list(freq), grid[row, ])))

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 27, 2026

@jonathon-love: The as.numeric-line you removed makes that the unit tests under R 4.6 fail.

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 27, 2026 •
edited
Loading 

Error (testdescriptives.R:211:5): Descriptives works old scenario
Error in `as.character.factor(X, ...)`: malformed factor
Backtrace:
    ▆
 1. ├─desc$frequencies[[1]]$asDF at testdescriptives.R:211:5
 2. ├─jmvcore:::`$.ResultsElement`(desc$frequencies[[1]], asDF) at testdescriptives.R:211:5
 3. └─jmvcore (local) `<fn>`()
 4.   └─jmvcore:::as.data.frame.Table(self)
 5.     └─base::unlist(as.list(column), use.names = FALSE)
 6.       └─base (local) URapply(x, as.character)
 7.         └─base::rapply(x, Fn, how = "list")
 8.           ├─base (local) `<fn>`(X, ...)
 9.           └─base::as.character.factor(X, ...)

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 27, 2026

Without as.numeric:

> descriptives(data, vars=c("w", "y", "z"), splitBy = "x",
                              freq=TRUE, median=TRUE, mode=TRUE, skew=TRUE,
                              kurt=TRUE, pc=TRUE)$frequencies[[1]]

 Frequencies of w                                    
 ─────────────────────────────────────────────────── 
   w    x    Counts     % of Total    Cumulative %   
 ─────────────────────────────────────────────────── 
   1    a    [table]       0.00000         0.00000   
        b    [table]       0.00000         0.00000   
        c    [table]       0.00000         0.00000   
   2    a    [table]       0.00000         0.00000   
        b    [table]       0.00000         0.00000   
        c    [table]       0.00000         0.00000   
   3    a    [table]       0.00000         0.00000   
        b    [table]       0.00000         0.00000   
        c    [table]       0.00000         0.00000   
 ─────────────────────────────────────────────────── 

There were 18 warnings (use warnings() to see them)

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 27, 2026

With as.numeric:

> descriptives(data, vars=c("w", "y", "z"), splitBy = "x",
                              freq=TRUE, median=TRUE, mode=TRUE, skew=TRUE,
                              kurt=TRUE, pc=TRUE)$frequencies[[1]]

 Frequencies of w                                     
 ──────────────────────────────────────────────────── 
   w    x    Counts      % of Total    Cumulative %   
 ──────────────────────────────────────────────────── 
   1    a    2.000000      16.66667        16.66667   
        b    1.000000       8.33333        25.00000   
        c    1.000000       8.33333        33.33333   
   2    a    1.000000       8.33333        41.66667   
        b    2.000000      16.66667        58.33333   
        c    1.000000       8.33333        66.66667   
   3    a    1.000000       8.33333        75.00000   
        b    1.000000       8.33333        83.33333   
        c    2.000000      16.66667       100.00000   
 ────────────────────────────────────────────────────

@sjentsch
Copy link
Copy Markdown
Member Author

sjentsch commented Feb 27, 2026

NB: The as.numeric works in 4.4 and 4.5 too.

@jonathon-love
Copy link
Copy Markdown
Member

jonathon-love commented Feb 27, 2026

roger that. i've brought that across now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonathon-love jonathon-love jonathon-love left review comments

@raviselker raviselker raviselker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sjentsch @jonathon-love @raviselker @shun2wang